Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrap Data.Aeson.Value in a newtype SafeValue #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mchaver
Copy link

@mchaver mchaver commented Oct 2, 2024

#20

This isn't complete but I wanted to make sure it is going in the right direction before completing it. Things to note:

  1. safeTo creates a SafeValue. I wasn't sure if safeFrom should take Value or SafeValue.
  2. I think the SafeValue constructor should not be exported. Just the type and the unSafeValue field.

@Vlix
Copy link
Owner

Vlix commented Oct 15, 2024

Ah, thank you for the effort. 🙏

I'll try to look at this when I have some free time (hopefully in the next month)

Copy link
Owner

@Vlix Vlix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SafeValue should also be exported from Data.SafeJSON. And we'll also need an instance SafeJSON SafeValue.

I don't think we'd have to hide the data constructor of SafeValue when exporting, since it's trivial for anyone to create a SafeValue from a regular Value anyway. (because of the instance SafeJSON Value)

@@ -1131,23 +1152,23 @@ instance SafeJSON (f (g a)) => SafeJSON (Compose f g a) where
version = noVersion

-- | @since 1.1.2.0
instance (SafeJSON (f a), SafeJSON (g a)) => SafeJSON (Sum f g a) where
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you comment out this instance?

@@ -328,17 +334,32 @@ setVersion = setVersion' (version @a)
-- should be hidden.
--
-- @since 1.0.0
removeVersion :: Value -> Value
removeVersion = \case
-- removeVersion :: SafeValue -> Value
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commented-out section can be removed, right?

@@ -268,17 +274,17 @@ getVersion _ = Nothing
-- "{\"~v\":0,\"~d\":\"test\"}"
--
-- @since 1.0.0
setVersion' :: forall a. SafeJSON a => Version a -> Value -> Value
setVersion' :: forall a. SafeJSON a => Version a -> SafeValue -> SafeValue
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I designed the setVersion functions to mainly be used to set the version when the value does not have one (taking a format from a third party who doesn't need to know about the versioning, for example)

So I'd say it would be more logical for the type to be Value -> SafeValue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants